Skip to content

feat: add support for setting the pushed oci image manifest annotations#2254

Merged
itowlson merged 1 commit intospinframework:mainfrom
rgl:add-registry-push-oci-image-manigest-annotations
Apr 3, 2024
Merged

feat: add support for setting the pushed oci image manifest annotations#2254
itowlson merged 1 commit intospinframework:mainfrom
rgl:add-registry-push-oci-image-manigest-annotations

Conversation

@rgl
Copy link
Contributor

@rgl rgl commented Jan 27, 2024

This closes #2236.

You can see how GitHub Container Registry shows an image without annotations at:

https://github.com/rgl/spin-http-ts-example/pkgs/container/spin-http-ts-example/171707227?tag=0.2.0

And the one with annotations at:

https://github.com/rgl/spin-http-ts-example/pkgs/container/spin-http-ts-example/171719572?tag=0.0.0-test1

This is how I've pushed it the image with my local spin version:

$ echo my-github-token-with-write-packages-scope | docker login ghcr.io -u rgl --password-stdin
$ ~/Projects/spin/target/debug/spin registry push --annotation "org.opencontainers.image.description=$(jq -r .description package.json)" ghcr.io/rgl/spin-http-ts-example:0.0.0-test1
Pushing app to the Registry...
Pushed with digest sha256:60373ae9983dac0356fc91b1ff016f3580fd7664a9061f71d8a5fd266c646a0c

@rgl rgl force-pushed the add-registry-push-oci-image-manigest-annotations branch 2 times, most recently from 1656611 to 12b60ea Compare January 27, 2024 12:21
@itowlson itowlson requested review from radu-matei and vdice January 28, 2024 19:53
Copy link
Collaborator

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Someone else will review the OCI side; I just had a couple of naming nits for the command code. We really appreciate the contribution!

use spin_oci::Client;
use std::{io::Read, path::PathBuf, time::Duration};

use super::new::ParameterValue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is used for template parameters. I realise that today it happens to have the right shape and parsing behaviour that you need for annotations, but we should not bind the two together. We should either:

  • Create a new type defined by shape and behaviour (a la EqualsSeparatedStringPair) in a shared location, and use that in both places; or
  • Create an Annotation type in this module, and use that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a common type at use spin_common::arg_parser::parse_kv, so I've rewritten to code to use it.

spin_build::build(&app_file, &[]).await?;
}

let annotations = self.annotations.map(|parameters| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The names parameters and parameter in this section are misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, indeed! In the meantime, this code no longer exists. Please see the new one.

@rgl rgl force-pushed the add-registry-push-oci-image-manigest-annotations branch 2 times, most recently from 2c8494b to 9ebda34 Compare January 29, 2024 07:15
Copy link
Collaborator

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update - this looks great! I had one suggestion for simplifying the Vector -> HashMap conversion but it's not a blocker. @vdice do you have a moment to look at the OCI side?

if self.annotations.is_empty() {
None
} else {
Some(self.annotations.iter().cloned().fold(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can do this with self.annotations.iter().cloned().collect() - no need for the mutable HashMap and the fold - an iterator of tuples can be collect-ed to a HashMap. To me this seems more concise and idiomatic than the mutating fold, although what you have works and this is not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there are duplicate --annotation arguments with the same key (e.g. --annotation a=a --annotation a=aa) won't that cause a panic?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, last one wins, same as with the fold.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice! Thank you for clarifying this!

I've updated the code, please check it now.

Copy link
Contributor

@vdice vdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @rgl!

Will hold off on merging in case any optional changes per #2254 (comment) come through.

@vdice
Copy link
Contributor

vdice commented Jan 29, 2024

@rgl sorry, I often forget this detail on new contributions: Thanks for completing the DCO sign-off; can you also please ensure that the commit(s) are GPG-signed? (When ready.)

@rgl rgl force-pushed the add-registry-push-oci-image-manigest-annotations branch from 9ebda34 to 8c2c5f6 Compare January 30, 2024 05:31
@rgl
Copy link
Contributor Author

rgl commented Jan 30, 2024

@vdice I think everything should be ready to go now :-)

@rgl
Copy link
Contributor Author

rgl commented Jan 30, 2024

@vdice please do not yet merge this until project-zot/zot#2210 is addressed.

I'm starting to think that we should have a way to set the image manifest annotations (what the current --annotation does in this MR) and the image config annotations. What do you guys think? Maybe have two arguments? --oci-image-manifest-annotation and --oci-image-config-annotation?

@vdice
Copy link
Contributor

vdice commented Jan 30, 2024

@rgl Would it be overkill to take the provided annotations and add them to both the manifest and image config?

A question: Refreshing my knowledge with the config spec, I see that the config object has a Labels field intended to capture "... arbitrary metadata for the container." Then, the config object, being a descriptor (if I'm understanding correctly), can also be assigned generic annotations. Which would we want to utilize here?

(As an aside, I built on this PR to play around a bit and I'm not immediately seeing generic annotations added to the config layer (here) show up as intended in the manifest json... so there may be a bit of further work either in our oci client or the underlying oci-distribution crate. I haven't yet tried adding metadata to the Config.Labels object on the config...)

@vdice
Copy link
Contributor

vdice commented Feb 20, 2024

Hi @rgl checking in on this one. Wondering what you were thinking re: #2254 (comment). Thanks!

@endocrimes
Copy link
Contributor

endocrimes commented Feb 20, 2024

It looks like docker adds them to the config block:

docker inspect my-org/my-project:

    "Config": {
            "Hostname": "",
            "Domainname": "",
            "User": "65532:65532",
            "AttachStdin": false,
            "AttachStdout": false,
            "AttachStderr": false,
            "Tty": false,
            "OpenStdin": false,
            "StdinOnce": false,
            "Env": [
                "SSL_CERT_FILE=/etc/ssl/certs/ca-certificates.crt",
                "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
            ],
            "Cmd": null,
            "Image": "",
            "Volumes": null,
            "WorkingDir": "/",
            "Entrypoint": [
                "/manager"
            ],
            "OnBuild": null,
            "Labels": {
                "org.opencontainers.image.created": "2024-02-19T16:28:43.720Z",
                "org.opencontainers.image.description": "",
                "org.opencontainers.image.licenses": "NOASSERTION",
                "org.opencontainers.image.revision": "ff68ce40b1d5e202a717483297db0eaa3bbacdc5",
                "org.opencontainers.image.source": "https://github.com/my-org/my-project",
                "org.opencontainers.image.title": "my-project",
                "org.opencontainers.image.url": "https://github.com/my-org/my-project",
                "org.opencontainers.image.version": "main"
            }
    },

@rgl
Copy link
Contributor Author

rgl commented Feb 20, 2024

@vdice sorry for the radio silence, but I'm afraid I was not yet able to dedicate quality time to understand the OCI image-spec in more details to be able to answer. I'm still interested in looking into this, hopefully soon :-)

@rgl
Copy link
Contributor Author

rgl commented Feb 20, 2024

@endocrimes do you known if that is aligned with the oci image-spec? I still didn't get a chance to spend quality time reading the specs, hopefully I can do it soon.

@itowlson
Copy link
Collaborator

itowlson commented Apr 1, 2024

@endocrimes sorry to hound but bark, bark

Copy link
Contributor

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy for this to land as is, and update later if needed for broader compatibility.

@itowlson
Copy link
Collaborator

itowlson commented Apr 1, 2024

Thanks @endocrimes! @rgl thanks for your patience - it's been a crazy busy time - could you rebase and resolve the merge conflict please? Then we can land this. Thanks!

@itowlson itowlson assigned rgl and itowlson and unassigned endocrimes Apr 1, 2024
@rgl rgl force-pushed the add-registry-push-oci-image-manigest-annotations branch from 8c2c5f6 to 9ecb814 Compare April 3, 2024 07:09
@rgl
Copy link
Contributor Author

rgl commented Apr 3, 2024

@itowlson no worries! I've rebased it.

@itowlson itowlson merged commit 65b2e45 into spinframework:main Apr 3, 2024
@itowlson
Copy link
Collaborator

itowlson commented Apr 3, 2024

Thank you @rgl! Glad to have landed this and thanks again for sticking with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spin registry push container image metadata

4 participants